Skip to content

fix: Avoid calling 'removeFromRunLoop' on 'NSStream' object when disconnecting from peer#503

Merged
pankcuf merged 1 commit into
developfrom
fix/dspeer-disconnect-crash
Jul 13, 2023
Merged

fix: Avoid calling 'removeFromRunLoop' on 'NSStream' object when disconnecting from peer#503
pankcuf merged 1 commit into
developfrom
fix/dspeer-disconnect-crash

Conversation

@tikhop

@tikhop tikhop commented Jul 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue being fixed or feature implemented

This PR fixed the infamous crash that occurs when the app attempts to disconnect from a peer. The main problem lies in certain scenarios where the -[DSPeer disconnectWithError:] method can be invoked on the same peer from multiple threads, leading to a crash in the NSStream object:

#1 Stack trace:
0. CFArrayGetCount //Array here is nil
1. _CFStreamClose
2. -[DSPeer disconnectWIthError:]
3. ...

#2 Stack trace:
0. CFArrayGetCount //Array here is nil
1. _CFStreamUnscheduleFromRunLoop
2. -[DSPeer disconnectWIthError:]

Currently, to disconnect from the peer, we have the following code:

[self.inputStream close];
[self.outputStream close]; // Crash happens here
[self.inputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];
[self.outputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes]; // Or here (randomly)

Looking inside of _CFStreamClose methods we can understand why the crash happens:

...
0x115c11704 <+220>: adrp   x24, 1386
0x115c11708 <+224>: ldr    x0, [x24, #0x648]         ; dictionary 
0x115c1170c <+228>: mov    x1, x19                   ; x1 is a key, x1 = <__NSCFInputStream: 0x6000016b07e0>
0x115c11710 <+232>: bl     0x115ba96b8               ; CFDictionaryGetValue
0x115c11714 <+236>: mov    x21, x0                   ; x21 = <__NSArrayI 0x600002412740> or nil when crash happens
0x115c11718 <+240>: ldr    x0, [x24, #0x648]         ; dictionary
0x115c1171c <+244>: mov    x1, x21                   ; x21 = <__NSArrayI 0x600002412740> 
0x115c11720 <+248>: bl     0x115ba96b8               ; CFDictionaryGetValue
0x115c11724 <+252>: mov    x22, x0
0x115c11728 <+256>: bl     0x115b6e508               ; CFArrayGetCount
0x115c1172c <+260>: mov    x20, x0
...
0x115c117ec <+452>: mov    x1, x19
0x115c117f0 <+456>: bl     0x115baa08c               ; CFDictionaryRemoveValue // Remove <__NSCFInputStream: 0x6000016b07e0> from the dict
0x115c117f4 <+460>: add    x21, x19, #0x38
0x115c117f8 <+464>: mov    x0, x21
...

Basically, it tries to access the value of the dictionary that was already removed. The same code we have inside _CFStreamUnscheduleFromRunLoop and it crashes at the same logical place.

To fix the issue, simple avoid calling:

[self.inputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];
[self.outputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];

Based on Apple docs the stream will be removed from run loop inside -[NSStream close] method:

Closing the stream terminates the flow of bytes and releases system resources that were reserved for the stream when it was opened. If the stream has been scheduled on a run loop, closing the stream implicitly removes the stream from the run loop. A stream that is closed can still be queried for its properties.

To prove that it is true, we can look inside of _CFStreamClose:

CoreFoundation._CFStreamClose:
    ...
    0x115a056e4 <+188>: mov    x0, x21
    **0x115a056e8 <+192>: bl     0x1159e2790               ; CFRunLoopSourceInvalidate**
    0x115a056ec <+196>: mov    x0, x21
    0x115a056f0 <+200>: bl     0x1159e7e50               ; CFRelease
    ...
    0x115a057ac <+388>: mov    x0, x23
    0x115a057b0 <+392>: mov    x1, x22
    **0x115a057b4 <+396>: bl     0x1159e1104               ; CFRunLoopRemoveSource**
    0x115a057b8 <+400>: mov    x0, x22
    0x115a057bc <+404>: bl     0x1159e7e50               ; CFRelease
    ...
    0x115a058a0 <+632>: ret

What was done?

  • Stop calling -[NSStream removeFromRunLoop:forMode] when peer gets disconnected

How Has This Been Tested?

To reproduce (simulate) the issue, simply try to call disconnect method from another thread right after we open the streams:

[self.inputStream open];
[self.outputStream open];
[NSThread detachNewThreadSelector:@selector(disconnect) toTarget:self withObject:nil];

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@tikhop
tikhop requested a review from pankcuf July 13, 2023 00:14
@tikhop
tikhop marked this pull request as ready for review July 13, 2023 00:18
@pankcuf
pankcuf merged commit 8ec6f94 into develop Jul 13, 2023
@pankcuf
pankcuf deleted the fix/dspeer-disconnect-crash branch July 13, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pankcuf pankcuf Awaiting requested review from pankcuf

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tikhop @pankcuf